Skip to content

apps/testing:move epoll, fatutf8 ... folders to the new fs folder#2963

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
txy-21:merge-fs-driver
Jan 21, 2025
Merged

apps/testing:move epoll, fatutf8 ... folders to the new fs folder#2963
xiaoxiang781216 merged 1 commit into
apache:masterfrom
txy-21:merge-fs-driver

Conversation

@txy-21

@txy-21 txy-21 commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

Summary

This is one step that merge test case as discussed in #2931.
Create fs folder and move {epoll, fatutf8, fasantest, fopencookie, fstest, mtd_config_fs, nxffs, smart, smart_test} folders to the new fs folder.

Impact

Only affect apps/testing folder

New folder :
fs

Move the following folder:
epoll
fatutf8
fasantest
fopencookie
fstest
mtd_config_fs
nxffs
smart
smart_test

Testing

CI test

@cederom cederom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @txy-21 :-)

@nuttxpr

nuttxpr commented Jan 21, 2025

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary and impact assessment, it lacks crucial information.

Here's what's missing:

  • Summary: Lacks clarity on why this change is necessary. Simply stating it's part of a larger effort isn't sufficient. What problem does this restructuring solve? How does it benefit the project? What functionality is being changed (even if it's just directory structure)? How exactly does the move work (e.g., are symbolic links used? Is it a pure move?). The linked PR provides context, but the PR itself should be self-contained.

  • Impact: While it mentions the affected folders, it lacks crucial "YES/NO" answers for most impact categories. Even if the answer is "NO," it should be explicitly stated. For example:

    • Impact on user: NO
    • Impact on build: Potentially YES (if any build scripts rely on the old paths). This needs clarification.
    • Impact on hardware: NO
    • Impact on documentation: Potentially YES (if any documentation refers to the old paths). This needs investigation and explicit statement.
    • Impact on security: NO
    • Impact on compatibility: Potentially YES. This change could break existing user setups or scripts that rely on the old paths. This needs investigation.
  • Testing: "CI test" is insufficient. While CI is important, the PR should provide specific examples of tests run locally to validate the change. What were the results before and after the move? Show concrete evidence that the restructuring didn't break anything. Including the build host and target information is also necessary.

In short, the PR needs to be more thorough and explicit in addressing all the requirements. It relies too heavily on the linked PR for context. Each PR should stand on its own.

Signed-off-by: tengshuangshuang <tengshuangshuang@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 8141e35 into apache:master Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants